Run Cygwin CI workflow commands in login shells #1709

EliahKagan · 2023-10-14T07:09:09Z

This passes --login to the bash shell used to run commands in the Cygwin environment on CI. This eliminates the need to work around a partly broken environment, and the extra code what was used to do that is accordingly removed. There are two benefits of this change:

The PATH is correct: Cygwin's /usr/local/bin and /usr/bin are present at the beginning of PATH. Otherwise, it is easy to get /usr/bin at the front, but rather involved to get /usr/local/bin to precede it. Because Python on Cygwin puts scripts/executables such as the upgraded pip and the pytest command in /usr/local/bin, it is valuable to have that directory in the PATH and best to have it before /usr/bin. (I have set CYGWIN_NOWINPATH to omit other directories, since finding any of the commands to be run in the Cygwin environment outside that environment is unintended.)
Every step automatically has correct temporary directories: When Cygwin commands were not being run in login shells, they didn't automatically get correct values for TMP and TEMP for their environment. To work around this, those environment variables were set globally, for every step. But that caused them to refer to nonexistent locations for steps such as actions/checkout. Most likely this would not cause any errors, but it did cause copious warnings about a nonexistent temporary directory, which risked obscuring other potentially important output. Now that Cygwin commands run in login shells, both the few non-Cygwin steps, and the steps run in the Cygwin environment, all get correct temporary directories (with TMP and TEMP set in the prewritten startup script the login shell uses).

A theoretical disadvantage of this is that login shells take slightly longer to start up, but that delay is insignificant in this application. A more significant disadvantage is that setting the -x shell option the way it was done before would produce a lot of noise at the beginning of the output for every command-running step. To work around that, -x is omitted from the value of shell and set -x is added at the end of the startup script for login shells, so it runs before each step's "payload" command, but without applying to the commands run in the startup script itself.

This passes --login to the bash shell used to run commands in the Cygwin environment on CI. This eliminates the need to work around a partly broken environment, and the extra code what was used to do that is accordingly removed. There are two benefits of this change: - The PATH is correct: Cygwin's /usr/local/bin and /usr/bin are present at the beginning of PATH. Otherwise, it is easy to get /usr/bin at the front, but rather involved to get /usr/local/bin to precede it. Because Python on Cygwin puts scripts/executables such as the upgraded "pip" and the "pytest" command in /usr/local/bin, it is valuable to have that directory in the PATH and best to have it before /usr/bin. (I have set CYGWIN_NOWINPATH to omit other directories, since finding any of the commands to be run in the Cygwin environment outside that environment is unintended.) - Every step automatically has correct temporary directories: When Cygwin commands were not being run in login shells, they didn't automatically get correct values for TMP and TEMP for their environment. To work around this, those environment variables were set globally, for every step. But that caused them to refer to nonexistent locations for steps such as actions/checkout. Most likely this would not cause any errors, but it did cause copious warnings about a nonexistent temporary directory, which risked obscuring other potentially important output. Now that Cygwin commands run in login shells, both the few non-Cygwin steps, and the steps run in the Cygwin enviroment, all get correct temporary directories (with TMP and TEMP set in the prewritten startup script the login shell uses). A theoretical disadvantage of this is that login shells take slightly longer to start up, but that delay is insigificant in this application. A more significant disadvantage is that setting the -x shell option the way it was done before would produce a lot of noise at the beginning of the output for every command-running step. To work around that, -x is omitted from the value of "shell" and "set -x" is added at the end of the startup script for login shells, so it runs before each step's "payload" command, but without applying to the commands run in the startup script itself.

Byron

That's so awesome! Great discovery!

I can sense that native Windows support is just around the corner :).

This makes a pip -> pip3 symlink in /usr/bin in a new step prior to the first step that runs the pip command. Using "pip3", "pip3.9", or a command like "python -m pip" would work, but this allows the Cygwin workflow to continue using the same installation commands as the main testing workflow. Adding this fixes two problems: 1. When the pip version installed by the python39-pip package is current, so that upgrading pip doesn't install any new commmand, no "pip" command is created in /usr/local. This has happened not to be the case for a long time, which is why the Cygwin workflow was able to pass. (That the recent failures started at the merge of gitpython-developers#1783 turns out to be a coincidence: rerunning jobs on prior commits has the failure, as does experimentally reverting it.) 2. Even when the pip version installed by python39-pip is behind the latest available version, pip is still used before being upgraded to check if setuptools is installed, to decide whether to upgrade it. This is to keep similar steps in the two testing workflows similar, since the Cygwin workflow only uses Python 3.9, which always has setuptools. Because pip was never in $PATH in that step, the Cygwin workflow wrongly refrained from trying to upgrade setuptools. When the "Update PyPA packages" step does find a newer version of pip to upgrade to, it installs it in /usr/local/bin, which we have in $PATH before /usr/bin, so the upgraded version, when present, will still be preferred in subsequent commands, as before. Running "pip" on Cygwin when it may not be in $PATH -- and, for one step, never is -- was a bug introduced in e8956e5 (gitpython-developers#1709). Before that, "pip" still was not always available, but it was not used. This change fixes the bug by making sure "pip" is always available.

This makes a pip -> pip3 symlink in /usr/bin in a new step prior to the first step that runs the pip command. Using "pip3", "pip3.9", or a command like "python -m pip" would work, but this allows the Cygwin workflow to continue using the same installation commands as the main testing workflow. Adding this fixes two problems: 1. When the pip version installed by the python39-pip Cygwin package is current, so that upgrading pip doesn't install any new commmand, no "pip" command is created in /usr/local. This has happened not to be the case for a long time, which is why the Cygwin workflow was able to pass. (That the recent failures started at the merge of gitpython-developers#1783 turns out to be a coincidence: rerunning jobs on prior commits has the failure, as does experimentally reverting it.) 2. Even when the pip version installed by python39-pip is behind the latest available version, pip is still used before being upgraded to check if setuptools is installed, to decide whether to upgrade it. This is to keep similar steps in the two testing workflows similar, since the Cygwin workflow only uses Python 3.9, which always has setuptools. Because pip was never in $PATH in that step, the Cygwin workflow wrongly refrained from trying to upgrade setuptools. When the "Update PyPA packages" step does find a newer version of pip to upgrade to, it installs it in /usr/local/bin, which we have in $PATH before /usr/bin, so the upgraded version, when present, will still be preferred in subsequent commands, as before. Running "pip" on Cygwin when it may not be in $PATH -- and, for one step, never is -- was a bug introduced in e8956e5 (gitpython-developers#1709). Before that, "pip" still was not always available, but it was not used. This change fixes the bug by making sure "pip" is always available.

EliahKagan marked this pull request as ready for review October 14, 2023 07:10

Byron approved these changes Oct 14, 2023

View reviewed changes

Byron merged commit 97c59ae into gitpython-developers:main Oct 14, 2023
8 checks passed

EliahKagan deleted the cygwin-ci-path branch October 14, 2023 11:01

EliahKagan mentioned this pull request Dec 23, 2023

Add missing pip in $PATH on Cygwin CI #1784

Merged

EliahKagan mentioned this pull request Dec 23, 2023

Revert "Don't install black on Cygwin" #1783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run Cygwin CI workflow commands in login shells #1709

Run Cygwin CI workflow commands in login shells #1709

EliahKagan commented Oct 14, 2023 •

edited

Loading

Byron left a comment

Run Cygwin CI workflow commands in login shells #1709

Run Cygwin CI workflow commands in login shells #1709

Conversation

EliahKagan commented Oct 14, 2023 • edited Loading

Byron left a comment

Choose a reason for hiding this comment

EliahKagan commented Oct 14, 2023 •

edited

Loading